-
Notifications
You must be signed in to change notification settings - Fork 137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use 'io/console', improve JRuby and Windows support, code coverage and add 'acceptance' tests #149
Conversation
…#ask We shouldn't rely on setting instance variables before a method invocation. Let's do it passing arguments only.
…Question itself It also simplifies HighLine#ask conditionals. At this same commit, remove the dependency on a @question instance variable.
... and fix tests accordling. Removing unecessary _state_ handling (inst vars) is a step toward a thiner HighLine class. Question shouldn't be a HighLine's _state_ because the **same** HighLine instance could be able to ask several questions. (At the previous code @question was a kind of _current_question_ state. But this is not really necessary currently.)
Some copyright notes above scoped class/module definitions were mixing up with the main scope (HighLine) class documentation.
With the constricted scope ```class HighLine::String < ::String``` we would have to do a ```include HighLine::StringExtensions``` The alternative fix here was to separate the scopes like ```ruby class HighLine class String < ::String ``` So we could use ```include StringExtensions```
So one could easily figure out if the "auto-detection" is not working.
There was a "warning" being issued.
... so it doesn't shadow an argument with same name.
…output Also save input and output as HighLine::Terminal instance variables reducing argument passing needs.
We currently didn't find a way to test Readline on JRuby and Rubinius without echoing to console. At MRI Ruby we set Readline.input and Readline.output to File instances.
…O, Tempfile and File We use StringIO, File and Tempfile at tests to mimetize STDIN and STDOUT (with io/console loaded). This modules add some methods (stubs) to give a more complete compatibility.
These 'acceptance' tests guide the user through some steps asking if they SEE what they should be seeing. It summarizes the answers and adds some debug environment information that can be sent to HighLine contributors. It'll be a complementary way of testing HighLine without the caveats of using a fake input/output object like StringIO as we do on automatic unit tests.
Rawline is a great gem, but I stilIl have much work to do with HighLine until I have a good 2.0.0 production version to ship. I think I'm going slowly here and this is because of lack of time. So I prefer to do one thing better than burn out trying to save the world! Perhaps when things get more "calm" here I can take another gem to contribute. |
@djberg96 could test this gist on your setup? |
@abinoam I put a comment on the gist. Definitely something wrong. I didn't realize Luis had turned over maintenance of rb-readline to someone else. There seem to be a lot of open issues. I also noticed that there's at least one fork of Rawline, by Zach Dennis, that seems to be getting updates. I've contacted him about taking over the project. |
I just read through the commits. This continues to be the most epically great refactoring effort I have ever seen. We must find ways to share this with the world: blog posts, conference talks, a book, or whatever. This is amazing stuff! Almost every commit I see something else that I really love:
Hooray! Super minor comments:
👍 x 1,000 Thank you so, so much!!! |
😊 Thank you very much for the feedback @JEG2 ! As I advance I'm getting more confident with the code base. When we ship the 2.0.0 we can think about spending some time to write some blog posts highlighting the most interesting things of this refactoring process. |
Use 'io/console', improve JRuby and Windows support, code coverage and add 'acceptance' tests
Merged! 👍 Closing. |
@djberg96 I have reported the findings in 2 issues on rb-readline |
@abinoam Thank you. And great job getting this change in! |
Any chance of a 2.0 prerelease gem? |
This is up to @abinoam, but my opinion is that we should do that around the time we feel the changes are mostly complete. |
Hi @djberg96, Perhaps we could just update the README to point that it is possible to run the "prerelease" version by pointing directly to the git repo at Gemfile as in: source "https://rubygems.org"
gem "highline", git: "https://github.com/JEG2/highline.git" What do you (@djberg96 and @JEG2) think about it? My fear about the prerelase is that people tend to "trust" them!!! No matter how you advise them. |
I prefer the README update, yes. |
@abinoam This is what prerelease gems are for - to get people to test drive them. If people trust them too much, that's not your problem. And, as you mentioned, they could still point at your repo with bundler, so it's not like depriving people of a prerelease gem will deter the die hards, but it does make it difficult to do standalone testing for small scripts. |
@djberg96 I may agree with the first part of what you said (that we're not saving the brave souls from using experimental code in production). But, could you explain more detailed the part of "make it difficult to do standalone testing for small scripts"? In my superficial thinking about the problem I find it easy to add the "git" option at the Gemfile. Or just clone the repo and do a NOTE: I'm not a native english speaker, so sometimes is hard for me to avoid sounding rude even if I try hard to sound polite. So... if unsure, I'm really interested in your suggestion. Just need to be sure I understand it clearly. |
@abinoam No worries, you didn't come off as rude, and I hope I didn't either. I suppose people could clone and install the repo, sure, so for a power user I guess there's no effective difference (except perhaps having to install whatever dev dependencies you have in the Rakefile). But, that's still more of a nuisance than just "gem install highline --prerelease". :) But, people don't use what they don't know about. I think (and hope) that creating a prerelease gem is more likely to get more people using it, because they're more likely to spot it with a gem search or gem update. I have no hard proof, or anything, just my instincts telling me it's more likely to be seen. Anyway, just my 0.02. It's your work, so do as you feel is best. |
Hi @djberg96 , I think you have convinced me. I've taken a look at http://guides.rubygems.org/patterns/#prerelease-gems With a prerelease gem we will have (summing up):
Confirm? So I agree with the prerelease gem. Let's see if @JEG2 sees this way too. |
I'll be disconnected for some time. Back at night. And thank you @djberg96 . |
@abinoam Yep, I think that's pretty much it. Maybe bundler requires a "pre" option of some sort, I don't remember right off, but I -think- what you have there works. |
I'm not against the idea. I would still probably wait until we're a bit closer to done, but this is @abinoam's show and he gets to make the call. |
@djberg96 I think bundler guess the "pre" mode by checking any non-numeric character after the digits of the (semantic) version.
|
@JEG2 you made blush 😊 !!! I have a midway solution. As code moved from place to place some of the documentation today is completely broken. Let me have some time to fix the documentation and then we release the "prerelease" version of the gem. What do you think about it? |
Sounds perfect to me. |
@abinoam How's it looking? |
I've had some personal issues that made me late. I think Ill have time to begin by thursday. |
Hi @JEG2 and all,
This PR brings the beginning of that idea of making some kind of interactive (acceptance) tests to HighLine.
Could you (all) test it with
rake acceptance
?Another thing I'm excited about is the use of 'io/console'.
Remember those comments #85 (comment) and #85 (comment) ?
I made some test here on my environment and I was able to run HighLine on JRuby using this new HighLine::Terminal::IOConsole mode.
I've put JRuby on our Travis matrix (allowed to fail) so we keep an eye on JRuby compatibility from now on.
Bumped up version to 2.0.0-develop.2
I'll let this PR open for some days to get some feedback (and perhaps add some commits more).
The complete Changelog below.
2.0.0-develop.2 / 2015-09-09
(by Abinoam P. Marques Jr. - @abinoam)
NOTES
This version brings greater compatibility with JRuby and Windows.
But we still have a lot of small issues in both platforms.
We were able to unify/converge all approaches into using io/console,
so we could delete old code that relied solely on stty, termios, java api and
windows apis (DL and Fiddle).
Another improvement is the beginning of what I called "acceptance tests".
If you type
rake acceptance
you'll be guided through some testswhere you have to input some thing and see if everything work as expected.
This makes easier to catch bugs that otherwise would be over-sighted.
CHANGES SUMMARY
methods for using at StringIO, File and Tempfile to help
on tests.
use HighLine::Terminal::IOConsole by default. This kind
of unifies most environments where HighLine runs. For
example, we can use Terminal::IOConsole on JRuby!!!
our first step to a more peaceful JRuby compatibility.
rake acceptance
to run themsome expected HighLine behavior is actually happening.
After that it gather some environment debug information,
so the use could send to the HighLine contributors in case
of failure.
ANY feedback is really appreciated!